-
Notifications
You must be signed in to change notification settings - Fork 39
[박지섭] sprint2 #102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[박지섭] sprint2 #102
The head ref may contain hidden characters: "Basic-\uBC15\uC9C0\uC12D-sprint2"
Conversation
- 코드리뷰를 통해 라우트가 아닌 빈 html로 연결하도록 수정하였습니다. - 미션2 진행을 위한 로그인, 회원가입 페이지도 같이 생성했습니다.
| </div> | ||
| </div> | ||
|
|
||
| <button type="submit" class="login__button">로그인</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로그인, 회원가입만 폰트가 풀리는 것 같습니다.. 무슨 문제인지 확인 부탁드립니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GANGYIKIM
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지섭님 2번째 미션 제출 수고하셨습니다.
코드를 이해하고 작성하시는 것 같아서 칭찬이 대부분이네요~
다음번 미션도 화이팅입니다!
-
유틸리티로 분리해서 사용하려고 스타일시트(typography)에 없는 폰트 속성이 있고, 계속 찾아보기 불편함이 생겼습니다. 이럴때는 폰트css를 제거한 후 각각의 스타일시트에서 개별적으로 값을 줘도 괜찮을지 궁금합니다.:
이러한 반응형 사이트에서 font 를 유틸리티 클래스로 선언하시고 작업하시는 것은 어려움이 있습니다. 예를 들어 text-lg라는 폰트가 모바일에 갔을 때 모든 페이지에서 동일하게 변경된다는 보장이 없다면 유틸리티 클래스를 tailwind 처럼 더 많이 작성하셔야 합니다~ 따라서 지금의 경우 그냥 개별로 값을 적용해주시는 것을 추천드려요. -
css파일의 import 방식을 수정해보았습니다. 배럴 파일처럼 구현하는게 맞았는지 지금처럼 index.css에거 관리하는게 맞았는지 궁금합니다. (index.css에 전부 import해서 관리할 때 유지보수 쪽 이슈가 있는지 궁금합니다.) 아니면 필요한 css파일만 관리하는 배럴파일을 따로 만들어서 관리하는게 더 좋은지 궁금합니다.:
지금과 같은 방식에서는 모든 페이지가 필요없는 css 를 불러와야합니다. 이는 불필요한 일이므로 이를 개선하는 방향으로 수정하시는 것이 좋습니다. 어떤 것이 좋은지는 취향 및 상황에 따라 다를 것 같은데 지금과 같이 component로 css를 분리해두셨으므로 페이지별로 css 파일을 생성하셔서 그곳에서 필요한 css 만을 import하시거나 각 페이지에서 필요한 css 만을 불러오시는 방식을 추천드립니다. -
login.css와 signup.css의 겹치는 속성이 많아서 해당 이슈는 미션3에 수정 후 pr 하도록 하겠습니다.:
좋습니다~ 관련해서 코멘트 남겨드렸지만, 지섭님께서 스스로 잘 해결하실 거라고 생각하니 편하게 진행해주세요!
| </div> | ||
| </div> | ||
|
|
||
| <button type="submit" class="login__button">로그인</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| </div> | ||
|
|
||
| <button type="submit" class="login__button">로그인</button> | ||
| </form> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 칭찬
form 태그로 관련된 요소만 묶어 주신 것 좋습니다~
| <label for="password">비밀번호</label> | ||
| <div class="input__wrapper"> | ||
| <input id="password" name="password" type="password" placeholder="비밀번호를 입력해 주세요" /> | ||
| <img src="./assets/icons/icon_invisible_eye.svg" alt="비밀번호 숨김" class="toggle-password" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❗️ 수정요청
해당 요소는 클릭 시 비밀번호가 보이고 안 보이게 만드는 요소가 될 것이므로 button요소로 작성하시는 것을 추천드려요.
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>판다마켓 - 회원가입</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 칭찬
자세하게 title 태그 적어주신 것 좋습니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 칭찬
important 빼주신것 너무 좋아요 👍
|
|
||
| /* Disable color */ | ||
| --disable-400: #9ca3af; | ||
| --disable-400: var(--secondary-400); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 칭찬
컬러 변수 선언하시고 사용하신 것 좋습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
로그인 페이지와 회원 가입 페이지가 디자인이 비슷한만큼 공통적으로 사용하는 css가 많아 보입니다.
이러한 중복을 줄이기 위해 공통 CSS 파일을 작성하시면 유지보수 및 가독성 측면에서 더 좋을 것 같습니다.
auth.css // login, signup 페이지에서 공통으로 쓰는 것들의 모음
login.css
signup.css
만약 login.css, signup.css 파일에서 작성되는 css가 너무 적다면, 즉 분리할 필요를 못 느끼신다면
auth.css 하나에서 전부 관리해도 좋을 것 같아요~

🐼 판다마켓 - 미션2
🌐 미션 배포 주소: https://harryseop-pandamarket.netlify.app/
기본 요구사항
체크리스트 [기본]
공통
로그인 페이지
회원가입 페이지
체크리스트 [심화]
주요 변경사항
주강사 님에게
typography)에 없는 폰트 속성이 있고, 계속 찾아보기 불편함이 생겼습니다. 이럴때는 폰트css를 제거한 후 각각의 스타일시트에서 개별적으로 값을 줘도 괜찮을지 궁금합니다.